Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Divider component to align with the Espresso by Frappe design system. The refactor removes the previous action-based implementation and introduces a more flexible slot-based approach that allows content to be positioned at start, center, or end positions within the divider.
Changes:
- Removed the old
actionprop andDividerActioninterface in favor of a more flexibleslotrender prop - Added support for custom
paddingandclassNameprops for better styling control - Simplified the implementation with clearer logic for horizontal/vertical orientations and slot positioning
- Added comprehensive test coverage and Storybook examples demonstrating all features
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/frappe-ui-react/src/components/divider/types.ts | Updated type definitions, removing DividerAction interface and action prop, adding slot, padding, and className props |
| packages/frappe-ui-react/src/components/divider/divider.tsx | Complete rewrite of component implementation with simplified slot-based approach and clearer styling logic |
| packages/frappe-ui-react/src/components/divider/tests/divider.tsx | New comprehensive test suite covering all component features including orientations, padding, slot positioning, and flexItem behavior |
| packages/frappe-ui-react/src/components/divider/divider.stories.tsx | New Storybook stories demonstrating all component variants and use cases including Timeline example |
| describe("Custom className", () => { | ||
| it("applies custom className", () => { | ||
| const { container } = render(<Divider className="custom-divider" />); | ||
| const hr = container.querySelector("hr"); | ||
|
|
||
| expect(hr).toHaveClass("custom-divider"); | ||
| }); |
There was a problem hiding this comment.
The test only verifies className application without a slot. Consider adding a test case that verifies custom className is applied when combined with a slot, since the className is applied to different elements depending on whether a slot is present (the hr element vs the wrapping div).
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| slot?: (args?: any) => ReactNode; |
There was a problem hiding this comment.
The slot prop uses a function that accepts args?: any but never passes any arguments when calling it. Consider either removing the parameter from the type definition or documenting what arguments might be passed in the future. Looking at other similar components in the codebase (like autocomplete, textInput), they use the same pattern but actually pass arguments to these render functions.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| slot?: (args?: any) => ReactNode; | |
| slot?: () => ReactNode; |
| position === "start" ? (isHorizontal ? "w-4" : "h-4") : "flex-1" | ||
| )} | ||
| /> | ||
| <span>{slot?.()}</span> |
There was a problem hiding this comment.
The slot function is called without any arguments, but the type signature suggests it could accept args?: any. This is inconsistent with the type definition. Either remove the parameter from the type definition, or if arguments are needed in the future, pass them here consistently.
| <span>{slot?.()}</span> | |
| <span>{slot?.(undefined)}</span> |
| it("applies padding to wrapper with slot", () => { | ||
| const { container } = render( | ||
| <Divider slot={() => "Text"} padding={20} /> | ||
| ); | ||
| const wrapper = container.querySelector("div"); | ||
|
|
||
| expect(wrapper).toHaveStyle({ | ||
| paddingTop: "20px", | ||
| paddingBottom: "20px", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests only verify padding behavior for horizontal dividers with slots (lines 81-91). There's no corresponding test for vertical dividers with slots and padding, which would verify that paddingLeft and paddingRight are applied correctly. Consider adding a test case for vertical orientation with slot and padding to ensure complete coverage of this feature combination.
| describe("Flex item", () => { | ||
| it("applies flex item classes when enabled", () => { | ||
| const { container } = render(<Divider flexItem />); | ||
| const hr = container.querySelector("hr"); | ||
|
|
||
| expect(hr).toHaveClass("self-stretch", "h-auto"); | ||
| }); |
There was a problem hiding this comment.
The test only verifies flexItem behavior without a slot. Consider adding a test case that verifies the flexItem behavior when combined with a slot, as the implementation applies flexClasses differently in those two scenarios (to hr vs to the wrapping div).
1 participant
Description
This PR updates Divider component based on Espresso by Frappe design.
Screenshot/Screencast
Checklist